Add caching support for IdentityProviderStorageProvider.getForLogin operations #2
Conversation
Closes #32573 Signed-off-by: Stefan Guilhen <[email protected]>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR implements comprehensive caching support for IdentityProviderStorageProvider.getForLogin() operations to optimize login page performance. The changes span multiple layers of the Keycloak architecture:
Core Implementation: The InfinispanIdentityProviderStorageProvider now caches getForLogin results using the existing IdentityProviderListQuery infrastructure, with cache keys based on realm ID, fetch mode, and organization ID. The caching follows established patterns in the codebase and includes sophisticated invalidation logic that only clears caches when changes actually affect login availability (enabled/disabled states, hideOnLogin flags, or organization associations).
Business Logic Enhancement: The IdentityProviderStorageProvider interface adds a new filter condition ensuring organization-associated identity providers are only included in login flows if they have the BROKER_PUBLIC configuration set to true. This maintains proper organizational isolation and security boundaries.
UI Layer Protection: OrganizationAwareIdentityProviderBean adds defensive idp.isEnabled() checks after the getForLogin() calls. These re-checks handle cases where cached IDPs might be wrapped in proxy objects that could potentially bypass the original enabled state filtering applied by the storage provider.
Test Coverage: A comprehensive test suite validates the caching behavior across different fetch modes (REALM_ONLY, ORG_ONLY, ALL) and verifies that cache invalidation occurs appropriately for login-affecting changes while avoiding unnecessary invalidations for other updates.
The implementation integrates seamlessly with Keycloak's existing caching infrastructure and organization features, providing performance benefits for login page rendering while maintaining data consistency and security requirements.
Confidence score: 4/5
• This PR appears safe to merge with proper caching implementation and comprehensive test coverage
• High confidence due to defensive programming measures and sophisticated invalidation logic that maintains cache consistency
• The OrganizationAwareIdentityProviderBean file needs more attention as the defensive enabled checks suggest potential issues with cached IDP wrapper behavior
4 files reviewed, 3 comments
| if (i >= 10) | ||
| idpRep.getConfig().put(OrganizationModel.BROKER_PUBLIC, Boolean.TRUE.toString()); | ||
| testRealm().identityProviders().create(idpRep).close(); | ||
| getCleanup().addCleanup(testRealm().identityProviders().get("alias")::remove); |
There was a problem hiding this comment.
logic: Cleanup reference uses incorrect alias - should be 'idp-alias-' + i instead of 'alias'
| getCleanup().addCleanup(testRealm().identityProviders().get("alias")::remove); | |
| getCleanup().addCleanup(testRealm().identityProviders().get("idp-alias-" + i)::remove); |
| idpRep.setDisplayName("Broker " + 20); | ||
| idpRep.setProviderId("keycloak-oidc"); | ||
| testRealm().identityProviders().create(idpRep).close(); | ||
| getCleanup().addCleanup(testRealm().identityProviders().get("alias")::remove); |
There was a problem hiding this comment.
logic: Same cleanup issue - incorrect alias reference
| getCleanup().addCleanup(testRealm().identityProviders().get("alias")::remove); | |
| getCleanup().addCleanup(testRealm().identityProviders().get("idp-alias-20")::remove); |
|
|
||
| Set<IdentityProviderModel> identityProviders = new HashSet<>(); | ||
| for (String id : cached) { | ||
| IdentityProviderModel idp = session.identityProviders().getById(id); |
There was a problem hiding this comment.
logic: using session.identityProviders().getById(id) calls the same provider recursively, which could lead to inconsistent caching behavior
| IdentityProviderModel idp = session.identityProviders().getById(id); | |
| IdentityProviderModel idp = idpDelegate.getById(id); |
2 participants
Test 2